Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[BUGFIX beta] Make computed.or return more like || #10259

Merged
merged 1 commit into from
Aug 3, 2015
Merged

[BUGFIX beta] Make computed.or return more like || #10259

merged 1 commit into from
Aug 3, 2015

Conversation

soulcutter
Copy link
Contributor

Given the recent deprecation of computed.any in favor of computed.or it
seems more-appropriate to return the last falsy value to enable
these computed properties to potentially be rendered directly in
templates without the need to be wrapped in {{#if}} blocks.

The behavior of computed.any was to return null, so this brings
computed.or more inline with replacing computed.any

The way javascript || works is that it returns the last value, so

undefined || null // => null
null || undefined // => undefined
null || false // => false

Edit: update to fix merge conflict, the way computed properties are declared/exported changed slightly

@wagenet
Copy link
Member

wagenet commented Jan 21, 2015

/cc @stefanpenner

@stefanpenner
Copy link
Member

hmm, I would like someone elses input on this @wycats r?

@soulcutter
Copy link
Contributor Author

Alternately computed.or could return null instead of false like computed.any did, which would make it more-useful for properties that end up getting rendered in templates.

@soulcutter
Copy link
Contributor Author

I'd like to resolve this one way or another - I think it's relatively important given the deprecation of any which suggests using or, which would be a poor substitute in situations where you are rendering the property and would prefer that it rendered nothing rather than false.

What are the objections or discussion points around this?

@stefanpenner
Copy link
Member

@soulcutter can you rebase this, we can get it in for 2.0.

Please rename it to [BUGFIX beta]

@soulcutter
Copy link
Contributor Author

Sure - when you say rename it, do you mean to prefix the commit message with that, or replace it completely with that? I assume "it" is the commit message

@stefanpenner
Copy link
Member

do you mean to prefix the commit message with that,

^^

@soulcutter
Copy link
Contributor Author

Done.

@stefanpenner
Copy link
Member

yay!

@soulcutter soulcutter changed the title Make computed.or falsy return value more like || [BUGFIX beta] Make computed.or return more like || Jul 8, 2015
@rwjblue
Copy link
Member

rwjblue commented Aug 2, 2015

@soulcutter - I'm sorry I missed your last comment. Looks like this needs another rebase.

@soulcutter
[BUGFIX beta] Make computed.or return more like ||
ad8e3f2 
Given the recent deprecation of computed.any in favor of computed.or it
seems more-appropriate to return the last falsy value to enable
these computed properties to potentially be rendered directly in
templates without the need to be wrapped in {{#if}} blocks.

The behavior of computed.any was to return null, so this brings
computed.or more inline with replacing computed.any
@soulcutter
Copy link
Contributor Author

Ok I rebased again. Fourth time is a charm? I hope this time is the last…

rwjblue added a commit that referenced this pull request Aug 3, 2015
@rwjblue
Merge pull request #10259 from soulcutter/computed-or-enhancement
a49c2eb 
[BUGFIX beta] Make computed.or return more like ||
@rwjblue rwjblue merged commit a49c2eb into emberjs:master Aug 3, 2015
@rwjblue
Copy link
Member

rwjblue commented Aug 3, 2015

Thank you!

@soulcutter soulcutter deleted the computed-or-enhancement branch August 3, 2015 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Feature Needs Team Discussion
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@soulcutter @wagenet @stefanpenner @rwjblue